docker: add healthcheck #1559
docker: add healthcheck #1559
Conversation
|
What's the benefit of having those health checks @MNThomson ? |
|
Could you please rebase the PR and resolve conflicts @MNThomson ? |
|
I can chime in regards to benefits of health check - it enables modern reverse proxies such as Træfik to postpone routing traffic to unhealthy containers and monitoring services to listen and send notifications when container becomes unhealthy. On Kubernetes environment, it is how It's actually a good thing. :) |
|
Thanks @flexchar . It seems that we could merge this once @MNThomson resolves the conflicts |
MNThomson
force-pushed
the
max/docker-healthcheck
branch
from
287ca1e
to
2a60f9a
Compare
haaawk
force-pushed
the
max/docker-healthcheck
branch
from
c33454e
to
ff6270e
Compare
|
|
||
| COPY --from=gosu /usr/local/bin/gosu /usr/local/bin/gosu | ||
| COPY --from=builder /etc/ssl/certs/ca-certificates.crt /etc/ssl/certs/ca-certificates.crt | ||
| COPY --from=builder /target/release/sqld /bin/sqld | ||
|
|
||
| USER root | ||
|
|
||
| HEALTHCHECK --interval=2s CMD /usr/local/bin/docker-healthcheck.sh |
There was a problem hiding this comment.
@athoscouto I would like to merge this PR. Just want to make sure this won't cause us any troubles on Fly. Please confirm.
|
|
||
| COPY --from=gosu /usr/local/bin/gosu /usr/local/bin/gosu | ||
| COPY --from=builder /etc/ssl/certs/ca-certificates.crt /etc/ssl/certs/ca-certificates.crt | ||
| COPY --from=builder /target/release/sqld /bin/sqld | ||
|
|
||
| USER root | ||
|
|
||
| HEALTHCHECK --interval=2s CMD /usr/local/bin/docker-healthcheck.sh |
There was a problem hiding this comment.
@MNThomson, sorry for late response - let's make HEALTHCHECK parameters configurable to simplify build of image with tuned parameters like this:
ARG HEALTHCHECK_INTERVAL=2s
ARG HEALTHCHECK_TIMEOUT=5s
ARG HEALTHCHECK_START_PERIOD=0s
ARG HEALTHCHECK_RETRIES=3
...
HEALTHCHECK --interval=$HEALTHCHECK_INTERVAL --timeout=$HEALTHCHECK_TIMEOUT --start-period=$HEALTHCHECK_START_PERIOD --retries=$HEALTHCHECK_RETRIES CMD ...There was a problem hiding this comment.
It would be even better to have a conditional check and add healthcheck only if those parameters are set
There was a problem hiding this comment.
A healthcheck should always run, please do not provide a footgun.
However this could be best managed by tuning it correctly by default.
There was a problem hiding this comment.
Perhaps worth a mention that a user can easily disable a healthcheck using docker CLI or docker compose or any other container execution framework. But if that one is correct in the first place, it is highly unlikely and provides a lot of value. :)
There was a problem hiding this comment.
Not every execution framework gives that option so it would be great to be able to control it with env vars
There was a problem hiding this comment.
TLDR; ship it without the healthcheck enabled by default, and update the dockerhub README with an example on how to enable it using docker compose.
Agreed with @haaawk on conditionally enabling, either via cmd and/or env. I'm fighting github actions right now at 2AM to get the go-gorm db wrapper shipped, and it was all because there was no way to configure a healthcheck, and the service was taking too long to come up.
For now I'm putting the pipeline to sleep to give the service enough time to come up, but it would be a nightmare if the health check always killed the container before the server fully started only because the host was slow. While I understand @Everspace's concern, neither to mysql nor postgres docker images have default healthcheck; but both images do offer facilities/cli commands to enable them if needed. I suspect this is to prevent containers from spontaneously ejecting or self-destructing even though the user hasn't set a health check.
Context
Added a Docker
HEALTHCHECKto both the production & dev Dockerfiles.Now ok, this bash is absolutely cursed and could be removed by just running
apt install curl. I didn't install curl since no other apt packages are installed in the runtime image and I'm assuming we want to keep the production image small. If that's acceptable though, adding curl is probably a better route.Test
{ "Status": "healthy", "FailingStreak": 0, "Log": [ { "Start": "2024-07-15T19:43:48.64977628-07:00", "End": "2024-07-15T19:43:48.703180793-07:00", "ExitCode": 0, "Output": "" } ] }Modifying
docker-health.shto point to a port that clearly isn't running or to/doesnotexistwill cause the healthcheck to fail (to simulate the/healthendpoint not responding):